build: make feature flags consistent #3921

camshaft · 2023-04-05T21:59:05Z

Description of changes:

We currently have 3 different ways to build the library: CMake, Make, and Cargo. Currently each method is required to maintain its own list of feature probes, which can easily get out of date.

This PR, instead, dynamically compiles a list of feature probes from the tests/features directory and uses the name of the file to define the symbol in the build (if the feature probe was successful). This makes adding a feature probe as easy as creating a .c and .flags file.

Call-outs:

While implemented the dynamic feature probes in Cargo, I noticed that the feature probes were actually broken so no features were being enabled there at all. This PR fixes that issue.

I also noticed that the fuzz builds were not picking up any feature probe flags. After this change, they now are correctly enabled:

...
clang -std=c99 -Wcast-qual -pedantic -Wall -Werror -Wimplicit -Wunused -Wcomment -Wchar-subscripts -Wuninitialized -Wshadow -Wcast-align -Wwrite-strings -fPIC -Wno-missing-braces -D_POSIX_C_SOURCE=200809L -O2 -I/codebuild/output/src079630060/src/github.com/aws/s2n-tls/test-deps/openssl-1.1.1/include/ -I/codebuild/output/src079630060/src/github.com/aws/s2n-tls/api/ -I/codebuild/output/src079630060/src/github.com/aws/s2n-tls -Wno-deprecated-declarations -Wno-unknown-pragmas -Wformat-security -D_FORTIFY_SOURCE=2 -fgnu89-inline -DS2N_FUZZ_TESTING=1 -Wstack-protector -fstack-protector-all -std=c99 -Wcast-qual -pedantic -Wall -Werror -Wimplicit -Wunused -Wcomment -Wchar-subscripts -Wuninitialized -Wshadow -Wcast-align -Wwrite-strings -fPIC -Wno-missing-braces -D_POSIX_C_SOURCE=200809L -O2 -I/codebuild/output/src079630060/src/github.com/aws/s2n-tls/test-deps/openssl-1.1.1/include/ -I/codebuild/output/src079630060/src/github.com/aws/s2n-tls/api/ -I/codebuild/output/src079630060/src/github.com/aws/s2n-tls -Wno-deprecated-declarations -Wno-unknown-pragmas -Wformat-security -D_FORTIFY_SOURCE=2 -fgnu89-inline -DS2N_FUZZ_TESTING=1 -Wstack-protector -fstack-protector-all -g3 -ggdb -fno-omit-frame-pointer -fno-optimize-sibling-calls -fsanitize-coverage=trace-pc-guard -fsanitize=address,undefined,leak -fprofile-instr-generate -fcoverage-mapping -Wno-strict-prototypes -DS2N_CLONE_SUPPORTED -DS2N_CPUID_AVAILABLE -DS2N_EXECINFO_AVAILABLE -DS2N_FALL_THROUGH_SUPPORTED -DS2N_FEATURES_AVAILABLE -DS2N_KYBER512R3_AVX2_BMI2_SUPPORTED  -DS2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH -DS2N_LIBCRYPTO_SUPPORTS_EVP_MD_CTX_SET_PKEY_CTX -DS2N_LIBCRYPTO_SUPPORTS_EVP_RC4  -DS2N_MADVISE_SUPPORTED  -DS2N_PLATFORM_SUPPORTS_KTLS -DS2N___RESTRICT__SUPPORTED -DS2N_STACKTRACE -Wcast-qual -pedantic -Wall -Werror -Wunused -Wcomment -Wchar-subscripts -Wuninitialized -Wshadow -Wcast-align -Wwrite-strings -fPIC -Wno-missing-braces -D_POSIX_C_SOURCE=200809L -O2 -I/codebuild/output/src079630060/src/github.com/aws/s2n-tls/test-deps/openssl-1.1.1/include/ -I/codebuild/output/src079630060/src/github.com/aws/s2n-tls/api/ -I/codebuild/output/src079630060/src/github.com/aws/s2n-tls -Wno-deprecated-declarations -Wno-unknown-pragmas -Wformat-security -D_FORTIFY_SOURCE=2 -DS2N_FUZZ_TESTING=1 -Wstack-protector -fstack-protector-all -Wno-strict-prototypes  -c -o s2n_kyber_512_evp.o s2n_kyber_512_evp.c
...

Testing:

Since the feature probing is handled in a consistent way, so is the logging of which features were enabled:

$ cmake . -Bbuild -DBUILD_TESTING=on -DCMAKE_BUILD_TYPE=Debug
-- Detected CMAKE_SYSTEM_PROCESSOR as x86_64
-- Detected 64-Bit system
-- LibCrypto Include Dir: /nix/store/gz31hsi6hdf0a2xzsznwfmv7vdw4ahcn-openssl-3.0.8-dev/include
-- LibCrypto Shared Lib:  /nix/store/np58sm45gngld2nnqjq6p532j3v2kbfl-openssl-3.0.8/lib/libcrypto.so
-- LibCrypto Static Lib:  crypto_STATIC_LIBRARY-NOTFOUND
-- Using libcrypto from the cmake path
-- CMAKE_AR found: /run/current-system/sw/bin/ar
-- CMAKE_RANLIB found: /run/current-system/sw/bin/ranlib
-- CMAKE_OBJCOPY found: /run/current-system/sw/bin/objcopy
-- feature S2N_CLONE_SUPPORTED: TRUE
-- feature S2N_CPUID_AVAILABLE: TRUE
-- feature S2N_EXECINFO_AVAILABLE: TRUE
-- feature S2N_FALL_THROUGH_SUPPORTED: TRUE
-- feature S2N_FEATURES_AVAILABLE: TRUE
-- feature S2N_KYBER512R3_AVX2_BMI2_SUPPORTED: TRUE
-- feature S2N_KYBER512R3_M256_INTRINSICS_SUPPORTED: TRUE
-- feature S2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH: TRUE
-- feature S2N_LIBCRYPTO_SUPPORTS_EVP_MD_CTX_SET_PKEY_CTX: TRUE
-- feature S2N_LIBCRYPTO_SUPPORTS_EVP_RC4: TRUE
-- feature S2N_LIBCRYPTO_SUPPORTS_KYBER512: FALSE
-- feature S2N_MADVISE_SUPPORTED: TRUE
-- feature S2N_MINHERIT_SUPPORTED: FALSE
-- feature S2N_PLATFORM_SUPPORTS_KTLS: TRUE
-- feature S2N___RESTRICT__SUPPORTED: TRUE
-- feature S2N_STACKTRACE: TRUE
-- feature S2N_KYBER512R3_AVX2_BMI2: TRUE
-- Configuring done
-- Generating done
-- Build files have been written to: build

You can also see the output in the flags.make:

$ cat build/CMakeFiles/s2n.dir/flags.make
# CMAKE generated file: DO NOT EDIT!
# Generated by "Unix Makefiles" Generator, CMake Version 3.24

# compile ASM with /run/current-system/sw/bin/cc
# compile C with /run/current-system/sw/bin/cc
ASM_DEFINES = -DS2N_CLONE_SUPPORTED -DS2N_CPUID_AVAILABLE -DS2N_EXECINFO_AVAILABLE -DS2N_FALL_THROUGH_SUPPORTED -DS2N_FEATURES_AVAILABLE -DS2N_KYBER512R3_AVX2_BMI2 -DS2N_KYBER512R3_AVX2_BMI2_SUPPORTED -DS2N_KYBER512R3_M256_INTRINSICS_SUPPORTED -DS2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH -DS2N_LIBCRYPTO_SUPPORTS_EVP_MD_CTX_SET_PKEY_CTX -DS2N_LIBCRYPTO_SUPPORTS_EVP_RC4 -DS2N_MADVISE_SUPPORTED -DS2N_PLATFORM_SUPPORTS_KTLS -DS2N_STACKTRACE -DS2N___RESTRICT__SUPPORTED -D_POSIX_C_SOURCE=200809L

ASM_INCLUDES = -I/home/cameron/Projects/aws/s2n-tls -I/home/cameron/Projects/aws/s2n-tls/api -isystem /nix/store/gz31hsi6hdf0a2xzsznwfmv7vdw4ahcn-openssl-3.0.8-dev/include

ASM_FLAGS = -g -pedantic -std=gnu99 -Wall -Wimplicit -Wunused -Wcomment -Wchar-subscripts -Wuninitialized -Wshadow -Wcast-align -Wwrite-strings -Wno-deprecated-declarations -Wno-unknown-pragmas -Wformat-security -Wno-missing-braces -Wno-strict-prototypes -Wa,--noexecstack -Werror -fvisibility=hidden -DS2N_EXPORTS -fPIC

C_DEFINES = -DS2N_CLONE_SUPPORTED -DS2N_CPUID_AVAILABLE -DS2N_EXECINFO_AVAILABLE -DS2N_FALL_THROUGH_SUPPORTED -DS2N_FEATURES_AVAILABLE -DS2N_KYBER512R3_AVX2_BMI2 -DS2N_KYBER512R3_AVX2_BMI2_SUPPORTED -DS2N_KYBER512R3_M256_INTRINSICS_SUPPORTED -DS2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH -DS2N_LIBCRYPTO_SUPPORTS_EVP_MD_CTX_SET_PKEY_CTX -DS2N_LIBCRYPTO_SUPPORTS_EVP_RC4 -DS2N_MADVISE_SUPPORTED -DS2N_PLATFORM_SUPPORTS_KTLS -DS2N_STACKTRACE -DS2N___RESTRICT__SUPPORTED -D_POSIX_C_SOURCE=200809L

C_INCLUDES = -I/home/cameron/Projects/aws/s2n-tls -I/home/cameron/Projects/aws/s2n-tls/api -isystem /nix/store/gz31hsi6hdf0a2xzsznwfmv7vdw4ahcn-openssl-3.0.8-dev/include

C_FLAGS = -g -pedantic -std=gnu99 -Wall -Wimplicit -Wunused -Wcomment -Wchar-subscripts -Wuninitialized -Wshadow -Wcast-align -Wwrite-strings -Wno-deprecated-declarations -Wno-unknown-pragmas -Wformat-security -Wno-missing-braces -Wno-strict-prototypes -Wa,--noexecstack -Werror -fvisibility=hidden -DS2N_EXPORTS -fPIC

# Custom flags: CMakeFiles/s2n.dir/pq-crypto/kyber_r3/KeccakP-1600-times4-SIMD256_avx2.c.o_FLAGS = -mavx2 -mavx -mbmi2

# Custom flags: CMakeFiles/s2n.dir/pq-crypto/kyber_r3/kyber512r3_cbd_avx2.c.o_FLAGS = -mavx2 -mavx -mbmi2

# Custom flags: CMakeFiles/s2n.dir/pq-crypto/kyber_r3/kyber512r3_consts_avx2.c.o_FLAGS = -mavx2 -mavx -mbmi2

# Custom flags: CMakeFiles/s2n.dir/pq-crypto/kyber_r3/kyber512r3_fips202x4_avx2.c.o_FLAGS = -mavx2 -mavx -mbmi2

# Custom flags: CMakeFiles/s2n.dir/pq-crypto/kyber_r3/kyber512r3_indcpa_avx2.c.o_FLAGS = -mavx2 -mavx -mbmi2

# Custom flags: CMakeFiles/s2n.dir/pq-crypto/kyber_r3/kyber512r3_poly_avx2.c.o_FLAGS = -mavx2 -mavx -mbmi2

# Custom flags: CMakeFiles/s2n.dir/pq-crypto/kyber_r3/kyber512r3_polyvec_avx2.c.o_FLAGS = -mavx2 -mavx -mbmi2

# Custom flags: CMakeFiles/s2n.dir/pq-crypto/kyber_r3/kyber512r3_rejsample_avx2.c.o_FLAGS = -mavx2 -mavx -mbmi2

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

dougch

Nice! Only other thought- it might be nice to put a couple sentences in DEVELOPMENT.md about how to add feature flags.

CMakeLists.txt

camshaft · 2023-04-06T23:30:50Z

it might be nice to put a couple sentences in DEVELOPMENT.md about how to add feature flags.

Yeah that's a good idea. Although I'd prefer to do it separately from the fix here.

lrstewart

This is a great idea!

Did you do any manual testing outside of the CI? You're making a lot of build changes, so I'm a little worried.

lrstewart · 2023-04-11T18:23:00Z

CMakeLists.txt

+    # Kyber Round-3 code has several different optimizations which require
+    # specific compiler flags to be supported by the compiler.
+    # So for each needed instruction set extension we check if the compiler
+    # supports it and set proper compiler flags to be added later to the
+    # Kyber compilation units.
+    if(${CMAKE_SYSTEM_PROCESSOR} MATCHES "^(x86_64|amd64|AMD64)$")


Wouldn't you expect the compilation attempt to fail if the flags aren't supported? Or are they just ignored?

I would expect it to fail, yeah. But I also didn't want to change the overall logic from what was there so I kept it.

lrstewart · 2023-04-11T18:25:04Z

CMakeLists.txt

 endif()
+feature_probe_result(S2N_KYBER512R3_AVX2_BMI2 ${S2N_KYBER512R3_AVX2_BMI2})


Am I missing something, or is this only supported via cmake, not for the bindings or make? Is that a problem?

But if that's right, then the same problem for S2N_STACKTRACE seems more serious...

camshaft · 2023-04-11T18:28:42Z

Did you do any manual testing outside of the CI? You're making a lot of build changes, so I'm a little worried.

That's fair. I went through all of our existing CI jobs to make sure that the features are enabled that should be enabled for that particular environment. I also did a bunch of manual review for each feature probe to make sure I didn't miss anything. If you have any other ideas I'm open!

camshaft · 2023-04-11T18:54:18Z

TODO:

Add a comment about S2N_STACKTRACE option in CMake
Implement the stacktrace definition in build.rs
Implement the stacktrace definition in Make

camshaft · 2023-06-01T18:15:24Z

~~My thinking is we open an issue to track adding stacktrace support to the Makefile later, as it's never had support for it so we're not any worse off today.~~

camshaft · 2023-06-01T18:43:10Z

I ended up just adding stacktrace support to the Makefile, since it does support it today; just no option to disable it:

s2n-tls/s2n.mk

Lines 187 to 190 in b9c4d60

    
           TRY_COMPILE_EXECINFO := $(call try_compile,$(S2N_ROOT)/tests/features/execinfo.c) 
        
           ifeq ($(TRY_COMPILE_EXECINFO), 0) 
        
           	DEFAULT_CFLAGS += -DS2N_STACKTRACE 
        
           endif

camshaft · 2023-06-01T21:43:15Z

The s2nIntegrationV2NixBatch job did succeed after a retry; not sure why the status isn't updating.

lrstewart · 2023-06-02T20:17:33Z

s2n.mk

+CFLAGS += $(SUPPORTED_FEATURES)
+DEFAULT_CFLAGS += $(SUPPORTED_FEATURES)
+CPPCFLAGS += $(SUPPORTED_FEATURES)


How were CFLAGS and CPPCFLAGS being set before? Were they being set before?

They weren't! Meaning anything using those wasn't getting any feature flags. I ran into this with the fuzz test while implementing #3798.

github-actions bot added the s2n-core team label Apr 5, 2023

camshaft force-pushed the feature-flag-fixes branch 3 times, most recently from 480bc43 to 2a56f02 Compare April 6, 2023 20:37

camshaft marked this pull request as ready for review April 6, 2023 21:03

camshaft requested review from lrstewart and dougch April 6, 2023 21:03

dougch approved these changes Apr 6, 2023

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

camshaft force-pushed the feature-flag-fixes branch from 2a56f02 to 948c19d Compare April 7, 2023 15:20

lrstewart reviewed Apr 11, 2023

View reviewed changes

build: make feature flags consistent

9227bd9

camshaft force-pushed the feature-flag-fixes branch from 948c19d to 9227bd9 Compare June 1, 2023 18:10

camshaft requested a review from lrstewart June 1, 2023 18:14

fix stacktrace support

403734b

fix fuzz feature flags

99a4e8d

camshaft mentioned this pull request Jun 2, 2023

feat: add checked return values diagnostic #3798

Merged

lrstewart approved these changes Jun 2, 2023

View reviewed changes

camshaft merged commit 6c38bde into aws:main Jun 2, 2023

camshaft deleted the feature-flag-fixes branch June 2, 2023 20:28

goatgoose mentioned this pull request Jun 24, 2023

fix: Add implicit gcc flag to all feature probes #4074

Merged

camshaft mentioned this pull request Apr 10, 2024

No CMake required for non-FIPS aws/aws-lc-rs#317

Merged

wombelix mentioned this pull request May 17, 2024

6 tests failing with Error Message: 'Certificate is untrusted' when compiling on RHEL 9 #4541

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: make feature flags consistent #3921

build: make feature flags consistent #3921

camshaft commented Apr 5, 2023 •

edited

Loading

dougch left a comment

camshaft commented Apr 6, 2023

lrstewart left a comment •

edited

Loading

lrstewart Apr 11, 2023

camshaft Apr 11, 2023

lrstewart Apr 11, 2023

camshaft commented Apr 11, 2023

camshaft commented Apr 11, 2023 •

edited

Loading

camshaft commented Jun 1, 2023 •

edited

Loading

camshaft commented Jun 1, 2023

camshaft commented Jun 1, 2023

lrstewart Jun 2, 2023 •

edited

Loading

camshaft Jun 2, 2023

		endif()
		feature_probe_result(S2N_KYBER512R3_AVX2_BMI2 ${S2N_KYBER512R3_AVX2_BMI2})

build: make feature flags consistent #3921

build: make feature flags consistent #3921

Conversation

camshaft commented Apr 5, 2023 • edited Loading

Description of changes:

Call-outs:

Testing:

dougch left a comment

Choose a reason for hiding this comment

camshaft commented Apr 6, 2023

lrstewart left a comment • edited Loading

Choose a reason for hiding this comment

lrstewart Apr 11, 2023

Choose a reason for hiding this comment

camshaft Apr 11, 2023

Choose a reason for hiding this comment

lrstewart Apr 11, 2023

Choose a reason for hiding this comment

camshaft commented Apr 11, 2023

camshaft commented Apr 11, 2023 • edited Loading

camshaft commented Jun 1, 2023 • edited Loading

camshaft commented Jun 1, 2023

camshaft commented Jun 1, 2023

lrstewart Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

camshaft Jun 2, 2023

Choose a reason for hiding this comment

camshaft commented Apr 5, 2023 •

edited

Loading

lrstewart left a comment •

edited

Loading

camshaft commented Apr 11, 2023 •

edited

Loading

camshaft commented Jun 1, 2023 •

edited

Loading

lrstewart Jun 2, 2023 •

edited

Loading